Skip to content

Comments

Fix #4656: Improve diagnostics for invalid proto files#72

Merged
daneshk merged 6 commits intoballerina-platform:mainfrom
ARYPROGRAMMER:fix/issue-4656-proto-diagnostic-improvements
Oct 22, 2025
Merged

Fix #4656: Improve diagnostics for invalid proto files#72
daneshk merged 6 commits intoballerina-platform:mainfrom
ARYPROGRAMMER:fix/issue-4656-proto-diagnostic-improvements

Conversation

@ARYPROGRAMMER
Copy link
Contributor

@ARYPROGRAMMER ARYPROGRAMMER commented Oct 2, 2025

Fixes ballerina-platform/ballerina-library#4656 - Improves error diagnostics when bal grpc command processes invalid proto files.

Problem: When processing invalid proto files, the command crashed with unhelpful IOException: Stream closed errors instead of displaying the actual protoc compiler error messages.

Root Cause: The handleProcessExecutionErrors() method was checking the process exit value before reading the error stream, causing the stream to close prematurely.

Solution:

  • Read error stream BEFORE checking process exit value to prevent stream closure
  • Added formatProtoError() helper method to format user-friendly error messages
  • Errors now include file name, line number, and clear descriptions from protoc

Examples

Before (Bad):

SEVERE: An error occurred when generating the proto descriptor.
io.ballerina.protoc.protobuf.exception.CodeGeneratorException: Invalid command syntax. Stream closed
    at io.ballerina.protoc.protobuf.utils.BalFileGenerationUtils.handleProcessExecutionErrors(...)
Caused by: java.io.IOException: Stream closed

After

Error: Failed to generate Ballerina files from proto definition.

Proto compilation failed with the following error:
user_service.proto:3:14: "User" is not defined.
user_service.proto:4:14: "Pagination" is not defined.

Please check your .proto file for syntax errors and undefined types.

Test Cases

  • Undefined message types: Shows clear "Type is not defined" messages
  • Syntax errors: Shows specific line/column with syntax issue
  • Valid protos: Continue to work correctly (regression test)

Checklist

@ARYPROGRAMMER
Fix #4656: Improve diagnostics for invalid proto files
ee27325 
Signed-off-by: Arya Pratap Singh <notaryasingh@gmail.com>
@CLAassistant
Copy link

CLAassistant commented Oct 2, 2025 •
edited
Loading

CLA assistant check
All committers have signed the CLA.

@ARYPROGRAMMER ARYPROGRAMMER mentioned this pull request Oct 9, 2025
Closed
@ARYPROGRAMMER
Copy link
Contributor Author

ARYPROGRAMMER commented Oct 12, 2025

@daneshk can you please review this

@daneshk
Copy link
Member

daneshk commented Oct 20, 2025

@daneshk can you please review this

@ARYPROGRAMMER Seems like we have duplicate methods. Shall we have one?

@ARYPROGRAMMER
Copy link
Contributor Author

ARYPROGRAMMER commented Oct 20, 2025

@daneshk can you please review this

@ARYPROGRAMMER Seems like we have duplicate methods. Shall we have one?

Give me some time I'll just see once

@ARYPROGRAMMER
fixture
9b89a24 
Signed-off-by: Arya Pratap Singh <notaryasingh@gmail.com>
@ARYPROGRAMMER
Copy link
Contributor Author

ARYPROGRAMMER commented Oct 20, 2025

@daneshk can you please review this

@ARYPROGRAMMER Seems like we have duplicate methods. Shall we have one?

yeah saw that lgtm now

@daneshk

@daneshk
Copy link
Member

daneshk commented Oct 21, 2025

@ARYPROGRAMMER I just went through the original code. We mistakenly duplicate whole BalFileGenerationUtils.java inside the utils package. we should remove the one in the utils package and use the one in the core package. Shall we do it in the same PR? So that we need one place do the change.

@ARYPROGRAMMER
Copy link
Contributor Author

ARYPROGRAMMER commented Oct 21, 2025

@ARYPROGRAMMER I just went through the original code. We mistakenly duplicate whole BalFileGenerationUtils.java inside the utils package. we should remove the one in the utils package and use the one in the core package. Shall we do it in the same PR? So that we need one place do the change.

yes absolutely we can do that. just check once my changes in few minutes

@ARYPROGRAMMER
remove duplicate
a0d0140 
Signed-off-by: Arya Pratap Singh <notaryasingh@gmail.com>
@ARYPROGRAMMER
Copy link
Contributor Author

ARYPROGRAMMER commented Oct 21, 2025 •
edited
Loading

@daneshk i have did exactly what you wanted. let's see if this this you like it

@ARYPROGRAMMER
checkstyle fix
a9f23f9 
Signed-off-by: Arya Pratap Singh <notaryasingh@gmail.com>
@ARYPROGRAMMER
Copy link
Contributor Author

ARYPROGRAMMER commented Oct 21, 2025

@daneshk this should fix the checkstyle error

@ARYPROGRAMMER
fix formatting
5cf0ebb 
Signed-off-by: Arya Pratap Singh <notaryasingh@gmail.com>
@ARYPROGRAMMER
Copy link
Contributor Author

ARYPROGRAMMER commented Oct 21, 2025

really worked hard on this, glad to contribute - hope this time everything is resolved

@codecov
Copy link

codecov bot commented Oct 21, 2025 •
edited
Loading

Codecov Report

❌ Patch coverage is 46.66667% with 16 lines in your changes missing coverage. Please review.
✅ Project coverage is 86.40%. Comparing base (abc9b95) to head (5cf0ebb).
⚠️ Report is 7 commits behind head on main.

Files with missing lines Patch % Lines
.../ballerina/protoc/core/BalFileGenerationUtils.java 46.66% 15 Missing and 1 partial ⚠️
Additional details and impacted files 
@@             Coverage Diff              @@
##               main      #72      +/-   ##
============================================
+ Coverage     84.39%   86.40%   +2.01%     
+ Complexity      656      655       -1     
============================================
  Files            57       56       -1     
  Lines          3312     3243      -69     
  Branches        376      361      -15     
============================================
+ Hits           2795     2802       +7     
+ Misses          414      338      -76     
  Partials        103      103

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@ARYPROGRAMMER
Copy link
Contributor Author

ARYPROGRAMMER commented Oct 21, 2025 •
edited
Loading

let me know if anything needed here, thanks @daneshk

@ARYPROGRAMMER
Copy link
Contributor Author

ARYPROGRAMMER commented Oct 22, 2025

can this be merged today @daneshk

daneshk
daneshk approved these changes Oct 22, 2025
View reviewed changes
Copy link
Member

@daneshk daneshk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@daneshk daneshk merged commit d599a8e into ballerina-platform:main Oct 22, 2025
4 of 5 checks passed
@daneshk
Copy link
Member

daneshk commented Oct 22, 2025

@ARYPROGRAMMER Thanks for the contribution.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@daneshk daneshk daneshk approved these changes

@lnash94 lnash94 Awaiting requested review from lnash94 lnash94 is a code owner

@shafreenAnfar shafreenAnfar Awaiting requested review from shafreenAnfar shafreenAnfar is a code owner

Assignees

No one assigned

Labels

hacktoberfest-accepted

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Improve diagnostics for invalid proto files

4 participants

@ARYPROGRAMMER @CLAassistant @daneshk @ThisaruGuruge